-
Notifications
You must be signed in to change notification settings - Fork 134
Refactor dbt utility for modularity #1095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5faf4b2 to
c066e86
Compare
72dbeef to
24565bd
Compare
636e109 to
d35ae67
Compare
d35ae67 to
099113a
Compare
|
I do not feel qualified to approve, but having the |
099113a to
56d83d6
Compare
3c9cfc9 to
f96ba4b
Compare
f96ba4b to
a942360
Compare
|
The build step is failing due to an unrelated dependency conflict, I think it's only triggering on this PR and not others because this one updates the docs folder |
12833fe to
c7b7ff9
Compare
c7b7ff9 to
0876c5d
Compare
0876c5d to
2bec4b2
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
128f40a to
54f5d49
Compare
4b82320 to
052dbb0
Compare
|
Finally fixed the build issue |
| @@ -0,0 +1,60 @@ | |||
| """Utility for running and logging output from dbt commands | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs are good, but ideally we want to be able to surface them on the docs website. Maybe add a hook into the autodocs on this page? https://move-coop.github.io/parsons/html/stable/utilities.html#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thiiink 8d56b8a should do this
shaunagm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor requests/questions but overall looks good. I know @matthewkrausse is looking forward to seeing this merged so hope to get it over the finish line soon and promise not to be a blocker on my end :)
parsons/utilities/dbt/dbt.py
Outdated
| def execute_dbt_command(self, command: str) -> Manifest: | ||
| """Runs dbt command and logs results after process is completed. | ||
|
|
||
| If raise_error is set, this method will raise an error if the dbt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is raise_error set? Where is it referenced? Is this an underlying dbt thing or is it part of this method/class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that was a line of obsolete documentation, removed with 10359ba
These changes uncouple the code for the dbt utility that runs a dbt command from the code that logs dbt commands. This allows for modular use of different loggers and a very flexible ability to customize logging. This addresses the main concerns from the original PR (#841).
Breaking changes
This PR does cause breaking changes with the previous implementation of the dbt utility, but those changes are well worth it for a much cleaner and clearer interface.
Using the new implementation looks like:
The prior implementation was potentially only compatible with Redshift, depending on how the user's dbt
profiles.yamlwas set up. The new implementation allows the dbt process to inherit the shell environment from the python parent process, and is therefore compatible with running dbt on any database, and the user can configure credential passing using environment variables (recommended) or any other method.Improved documentation
There is now much more thorough documentation throughout the 4 modules containing the dbt utility code.
TO DO: